Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

detectOpenHandles imply runInBand #8283

Merged
merged 4 commits into from
Apr 7, 2019

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Apr 7, 2019

Summary

detectOpenHandles makes no sense without runInBand, because it cannot detect leaks in workers.
I've also changed shouldRunInBand to just accept the config object, otherwise it would have lots of params.

Test plan

Added one test.each line. Also tested manually.

@jeysal jeysal requested review from thymikee and SimenB April 7, 2019 15:40
) {
/**
// detectOpenHandles makes no sense without runInBand, because it cannot detect leaks in workers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it cannot detect leaks in workers

Is this a limitation of async_hooks API?

Anyway, since detectOpenHandles flag is handled before initializing TestScheduler, how about moving this logic there and adding a proper error message so users are aware? We need to alarm users about that anyway.

I'm good passing whole config to shouldRunInBand 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a limitation of async_hooks API?

I mean, you could certainly make leak detection work in each worker, but I don't think it's worth it - a detectOpenHandles debug run doesn't need performance through parallelization.

adding a proper error message so users are aware? We need to alarm users about that anyway.

Not sure I understand correctly, do you want to error if detectOpenHandles without runInBand is used instead of just activating runInBand implicitly? I think forcing users to specify both would be quite annoying. Printing "did not exit within 1 second, please use --detectOpenHandles --runInBand" also doesn't look good :/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the second thought, we can hide it from the user. Would be nice to add a description note that adding this flag will make tests running serially

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to docs & yargs 👍

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants